feat: Integrating filters into discovery #5034
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR refactors discovery constructor logic to conditionally enable and parse filters when the FilterFlag experiment is active, introduces a new public Changes
Sequence DiagramsequenceDiagram
participant Discovery
participant Filter
participant Component Creation
Note over Discovery: filterFlagEnabled = true
Discovery->>Discovery: processFile(path)
alt filterFlagEnabled is true
Discovery->>Discovery: Skip exclude pattern checks
Discovery->>Component Creation: createComponentFromPath()
alt Hidden directory allowed
Component Creation->>Filter: Evaluate filters
alt Filters match
Filter-->>Component Creation: ✓ Include
Component Creation-->>Discovery: Return component
else Filters don't match
Filter-->>Component Creation: ✗ Skip
Component Creation-->>Discovery: Return nil
end
else Hidden directory not allowed
Component Creation-->>Discovery: Skip component
end
else filterFlagEnabled is false
Discovery->>Discovery: Apply standard exclude patterns
Discovery->>Component Creation: Regular component creation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📚 Learning: 2025-08-19T16:05:54.723ZApplied to files:
📚 Learning: 2025-02-10T13:36:19.542ZApplied to files:
📚 Learning: 2025-07-03T22:05:07.356ZApplied to files:
🧬 Code graph analysis (5)test/integration_hcl_filter_test.go (1)
internal/filter/filters.go (2)
internal/discovery/constructor.go (4)
internal/discovery/discovery.go (5)
internal/discovery/discovery_test.go (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (20)
Comment |
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
3 times, most recently
from
c574388 to
4de37b3
Compare
yhakbar
force-pushed
the
feat/integrating-filters-into-discovery
branch
from
d4e5a07 to
474e9ed
Compare
yhakbar
force-pushed
the
chore/disable-experimental-ci-ignore
branch
3 times, most recently
from
b684454 to
7c30c1b
Compare
yhakbar
force-pushed
the
feat/integrating-filters-into-discovery
branch
from
845b4f8 to
652c6dc
Compare
chore: Renaming `RequiresHCLParsing` to `RequiresDiscovery` as that captures the essence of the check better chore: Testing exclude by default with filters
yhakbar
force-pushed
the
feat/integrating-filters-into-discovery
branch
from
652c6dc to
cad37ef
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| return nil | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Unnecessary Filter Evaluation on Empty Filters Flag
Missing check for empty filters before evaluation. When d.filterFlagEnabled is true but d.filters is empty (e.g., when filter flag experiment is enabled but no filters are provided), the code at line 771 will still attempt to evaluate filters unnecessarily. The condition if _, requiresParsing := d.filters.RequiresDiscovery(); !requiresParsing will be true when filters is empty (returns false), causing d.filters.Evaluate() to be called on every component even when there are no filters to apply. This is inconsistent with the pattern used elsewhere in the code (lines 1041 and 1120) where len(d.filters) > 0 is checked before evaluation. While not causing incorrect behavior (empty filters return input unchanged), this creates unnecessary overhead during file processing.
| cleanDir := util.CleanPath(canonicalDir) | ||
| for part := range strings.SplitSeq(cleanDir, "/") { | ||
| if part == config.StackDir { | ||
| allowHidden = true |
There was a problem hiding this comment.
I think this code can be simplified into separate function called isInStackDirectory(path)
Description
Starts integrating filters into the logic for discovery so that we can prevent discovery of components when certain filters are set.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Added / Removed / Updated [X].
Migration Guide
Note
Integrates filter-aware discovery (with experiment flag), refactors constructor, and renames filter parsing requirements to "RequiresDiscovery" with updated errors and tests.
WithFilterFlagEnabledandWithFilters; positive filters setexcludeByDefault.NewDiscoverytoconstructor.go; wire filters intoNewForDiscoveryCommandandNewForHCLCommand.createComponentFromPathhelper; minor concurrency/walk refactors.RequiresHCLParsing→RequiresDiscoveryacross AST andFilters.FilterQueryRequiresHCLParsingErrorwithFilterQueryRequiresDiscoveryErrorand update messages.Written by Cursor Bugbot for commit cad37ef. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests